fix(authz): fall back to body principal in _register_in_auth - #373
fix(authz): fall back to body principal in _register_in_auth#373alvinkam2001 wants to merge 6 commits into
Conversation
asherfink
left a comment
There was a problem hiding this comment.
verified a few things beyond the happy path:
- scope is right.
_register_in_authonly runs on the genuine first-time create branch, so the gitea/register-build path (agent already exists at deploy time, so register hits an update branch) never hits this. this only bites the older helm-install/self-register model, which is Rocket. so the blast radius matches the description. - middleware-takes-precedence is preserved and now test-guarded, so an authenticated caller can't slip a body principal in to reattribute ownership. good, that's the property that matters most.
register_resourcestill can't run unless the route's createcheckpassed (enforce_ownership gates both), so no create-gate bypass.- leaving register-build alone is correct (not whitelisted, middleware always set).
- checked logging since it's forwarding a principal further: the one new log line carries only agent_id, and the authz gateway's error handler logs the truncated response text, not the request payload. principal_context can carry an api_key (the auth cache explicitly refuses to cache principals that do), so keep it out of any debug logging on these paths, but nothing in this PR logs it. clean.
one non-blocking note inline: a register/deregister principal asymmetry in the compensation path. good to merge once you've glanced at it.
| await self.authorization_service.register_resource( | ||
| AgentexResource.agent(agent_id) | ||
| AgentexResource.agent(agent_id), | ||
| principal_context=principal_context, |
There was a problem hiding this comment.
one asymmetry worth closing: register_resource now runs with the resolved (body) principal, but the compensating _safe_deregister still calls deregister_resource(agent(agent_id)) with no principal, so on the whitelisted pod path it deregisters with the None middleware principal. if a multi-pod cold start races and one loses on DuplicateItemError, its _safe_deregister fires with None and (assuming deregister validates the principal the same way register/grant do, per the #292 rationale that a None principal 422s) silently fails and swallows, leaving an orphaned ownership tuple for an agent id that was never persisted. narrow (needs the parallel first-create race on the older helm path) and not a security issue since it's the same account, but worth threading the resolved principal into _safe_deregister so register/deregister stay symmetric. if deregister ignores the principal server-side, disregard.
| async def _safe_deregister( | ||
| self, agent_id: str, principal_context: Any = None | ||
| ) -> None: |
There was a problem hiding this comment.
_safe_deregister now always passes principal_context=principal_context to deregister_resource. When called from delete (line 440) as _safe_deregister(agent.id), the default None is forwarded. AuthorizationService.deregister_resource uses an Ellipsis sentinel (principal_context=...) to mean "fall back to middleware"; receiving None instead causes it to pass None straight to the gateway, silently bypassing the middleware principal on every delete-path deregistration. Pre-PR, the call carried no principal_context kwarg at all, so the middleware principal was used correctly. Propagating the same ... sentinel as the default restores that behaviour.
| async def _safe_deregister( | |
| self, agent_id: str, principal_context: Any = None | |
| ) -> None: | |
| async def _safe_deregister( | |
| self, agent_id: str, principal_context: Any = ... | |
| ) -> None: |
Prompt To Fix With AI
This is a comment left during a code review.
Path: agentex/src/domain/use_cases/agents_use_case.py
Line: 44-46
Comment:
`_safe_deregister` now always passes `principal_context=principal_context` to `deregister_resource`. When called from `delete` (line 440) as `_safe_deregister(agent.id)`, the default `None` is forwarded. `AuthorizationService.deregister_resource` uses an Ellipsis sentinel (`principal_context=...`) to mean "fall back to middleware"; receiving `None` instead causes it to pass `None` straight to the gateway, silently bypassing the middleware principal on every delete-path deregistration. Pre-PR, the call carried no `principal_context` kwarg at all, so the middleware principal was used correctly. Propagating the same `...` sentinel as the default restores that behaviour.
```suggestion
async def _safe_deregister(
self, agent_id: str, principal_context: Any = ...
) -> None:
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Can you address this?
There was a problem hiding this comment.
The issue is clear. The delete call site at line 440 still calls _safe_deregister(agent.id) with no principal_context, so it gets None instead of ..., bypassing the middleware fallback in AuthorizationService.deregister_resource.
The fix is exactly what was suggested — change the default from None to ...:
| async def _safe_deregister( | |
| self, agent_id: str, principal_context: Any = None | |
| ) -> None: | |
| async def _safe_deregister( | |
| self, agent_id: str, principal_context: Any = ... | |
| ) -> None: |
This restores pre-PR behaviour for the delete path (middleware principal is used via the Ellipsis sentinel) while still allowing _register_in_auth's compensation calls to forward the resolved body principal explicitly when needed.
Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.
Summary
The whitelisted
/agents/registerpath clears the middleware principal. The pod SDK ships the manifest-declared identity in the request body, but_register_in_authwas still reading only from the middleware and silently skipping ownership registration. This threads the body-supplied principal into the use case as a fallback, mirroring what #325 (cae5f94) already did for the route'scheck/grantfallback.Symptom pre-fix: agent registration succeeds, no error logs, WARN
Skipping authorization registration for agent: no creator resolvablefires atagents_use_case.py:83, and the agent has no owner tuple inagentex_permissions— invisible to the SGP UI listing.Root cause
Bisected across three commits on the same code path:
14796e9(#270)_register_in_auth, keyed offself.authorization_service.principal_context.63f89e7(#292)_has_resolvable_creatorguard — skipscheck/grantwhen middleware principal is missing (i.e. the whitelisted pod path).cae5f94(#325)principal_contextforcheck/grant.cae5f94recovered ownership grants for authenticated CLI/UI callers via the route layer, but the use case'sregister_resourcecall inside_register_in_authwas still reading only from the middleware. Result: every pod-based deploy since Jun 3 that lands on the whitelisted route lost its owner tuple.What this change does
agentex/src/domain/use_cases/agents_use_case.py_register_in_authaccepts an optionalbody_principal_contextand uses it as fallback when the middleware principal isn't resolvable. Passes the resolved principal explicitly toregister_resource(..., principal_context=...)so it doesn't silently default to the (None) middleware principal._has_resolvable_creatorstaticmethod (same shape as the one inagents.py:53).register_agentaccepts and forwardsbody_principal_context.agentex/src/api/routes/agents.pybody_principal_context=request.principal_contextwhen calling the use case, matching the precedence already applied tocheck/grant.agentex/tests/integration/use_cases/test_agent_authz_dual_write.pytest_create_falls_back_to_body_principal_when_middleware_missing— covers the whitelisted pod path with a body-supplied service account.test_middleware_principal_takes_precedence_over_body— regression guard confirming authenticated CLI/UI callers keep using the middleware identity.What this change does NOT do
register_buildis intentionally left alone. That route isn't whitelisted, so the middleware principal is always set for legitimate callers — there's no trigger for the fallback there. Keeping the fix scoped to the actual regression._has_resolvable_creatoris now duplicated betweenagents.py(route) andagents_use_case.py(use case). De-duping into a shared util would be an unrelated refactor and would need to sit somewhere neutral (not inapi/, since the domain shouldn't import from it). Happy to do that in a follow-up if a reviewer prefers.Symptom (from Rocket beta)
The manifest for this agent declared a valid service account:
The SDK correctly base64-encodes this into
AUTH_PRINCIPAL_B64and ships it in the register body'sprincipal_contextfield (seescale-agentex-python/src/agentex/lib/utils/registration.py). The service was just not reading it in_register_in_auth.Manual backfill of the
agentex_permissionsrow (viaPOST /private/v5/agentex/permissionson egp-api-backend) restored visibility, confirming the SGP permissions row is what was missing.Test plan
test_create_falls_back_to_body_principal_when_middleware_missingtest_middleware_principal_takes_precedence_over_bodyTestAgentRegisterOnCreatestill pass locally (9/9 passed, 6 Docker-only tests deselected)agentex_permissionsrow is written automatically (no manual backfill required) and the agent shows up in the SGP UI listingGreptile Summary
This PR fixes agent ownership registration on the whitelisted
/agents/registerpath:_register_in_authnow acceptsbody_principal_contextand falls back to it when the middleware principal is unresolvable, then forwards the resolved principal explicitly toregister_resourceand symmetrically to_safe_deregisteron compensation.agents_use_case.py:_register_in_authgrows abody_principal_contextfallback and returns(registered, resolved_principal)so caller sites can propagate the same principal to_safe_deregister; a new_has_resolvable_creatorstatic method consolidates the identity check.agents.py: The route passesbody_principal_context=request.principal_contexttoregister_agent, mirroring the already-existing body fallback applied tocheck/grant.Confidence Score: 4/5
_safe_deregistersignature change silently breaks the delete-path deregistration by forwardingNoneto the auth gateway instead of the middleware principal._safe_deregisterfunction now defaultsprincipal_contexttoNoneand unconditionally passes it as a kwarg toderegister_resource. The authorization service distinguishesNonefrom the Ellipsis sentinel — receivingNoneskips the middleware fallback and forwardsNonestraight to the gateway. Thedeletecall site at line 440 invokes_safe_deregister(agent.id)with noprincipal_contextargument, so every soft-delete deregistration now sendsNoneto the auth gateway rather than the authenticated caller's identity. This regression exists on a path that ran correctly before this PR._safe_deregisterand its call site indeleteat line 440.Important Files Changed
_register_in_authnow accepts and falls back tobody_principal_contextwhen middleware principal is unresolvable, and forwards the resolved principal symmetrically to_safe_deregister. The_safe_deregisterdefault ofprincipal_context=None(forwarded as-is toderegister_resource) breaks the delete path where the call site passes no principal — the service's Ellipsis sentinel no longer applies, soNonereaches the gateway.body_principal_context=request.principal_contextinto the use case, mirroring the already-existing route-layer check/grant fallback. No logic changed in the route itself._record_existenceside-effect is updated to accept the newprincipal_contextkwarg, addressing the previous outside-diff comment.Sequence Diagram
sequenceDiagram participant Pod as Pod SDK (whitelisted path) participant Route as agents.py route participant UC as AgentsUseCase participant AuthSvc as AuthorizationService Pod->>Route: "POST /agents/register<br/>(body: principal_context = SA from manifest)" Note over Route: middleware principal = None Route->>Route: _has_resolvable_creator(None) → False Route->>Route: "principal_context = request.principal_context (body SA)" Route->>Route: "enforce_ownership = True" Route->>AuthSvc: "check(agent:*, create, principal_context=body_SA)" Route->>UC: "register_agent(..., body_principal_context=body_SA)" UC->>UC: "_register_in_auth(agent_id, body_principal_context=body_SA)" UC->>UC: middleware principal not resolvable → fallback to body_SA UC->>AuthSvc: "register_resource(agent, principal_context=body_SA)" AuthSvc-->>UC: ok UC->>UC: agent_repo.create(agent) alt DuplicateItemError UC->>AuthSvc: "deregister_resource(agent, principal_context=body_SA)" UC->>UC: adopt existing row end UC-->>Route: agent_entity Route->>AuthSvc: "grant(agent, principal_context=body_SA)" Route-->>Pod: RegisterAgentResponseReviews (6): Last reviewed commit: "Merge branch 'main' into akam/fix-authz-..." | Re-trigger Greptile